-
Notifications
You must be signed in to change notification settings - Fork 782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate language modes to CSV #1528
Migrate language modes to CSV #1528
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
This is ready for review again. |
Snippets apparently uses language_ids from language_modes.py, so it might be worth making sure that the snippets system still works. The following code for dealing with snippets will not get reran on updating the CSV.
I could address this by creating a callback registration mechanism inside language_modes.py. |
This list is alphabetized.
We could declare an action that returns the list of languages? |
Yes, but that would not cause the snippets code to update in response to the csv updating. |
Gotcha. Not sure if we have an example of this (I couldn't find one on a cursory search of community), but an action that triggers when the list is updated seems reasonable. |
I plan on writing code to allow registering callbacks to handle language mode updates. |
for more information, see https://pre-commit.ci
I theoretically have the code working, but I do not use the snippets system, so I will need to learn how it works before I can test it properly. |
I did further testing of the snippet system, and it seems to be working with my changes. I consider the pull request ready for review, but someone more familiar with the snippet system than myself should make sure that my changes did not break anything. |
Notes form their community session. We're not sure that introducing a event system especially for this use case is desirable. |
I am going to be busy for a while starting next week, so I am fine with another developer taking this PR. |
Okay I looked into the snippet dependency on language ids and the result is not that good. Basically the only reason why I'm using the language ids from the language modes file and not the actual snippets themselves is because of performance. Dynamically creating contexts is problematic for Talon. Even to the extent that it have been suggested we should remove the custom snippet dir setting. The current implementation creates all the contexts once before Talon is started/ready community/core/snippets/snippets.py Lines 32 to 36 in 9af1757
Issue talking about the problem with updating snippet contexts The implementation in this pull request removes the performance gain in creating the contexts before Talon starts. In that case we could just get the language ids from the snippet files themself. I can't really say how much of a problem it is to create the context dynamically, but I definitely feel that if we are going to create them dynamically we should not do it based upon the language mode file, but the snippet files themself. |
|
||
# Create a context for each defined language | ||
for lang in language_ids: | ||
ctx = Context() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we decide to keep this implementation we definitely should check if the context already exists before creating a new one
My preferred solution would be to get the language identifiers from the snippet files themselves directly when the file is read so the context would be created before Talon is ready. The only scenario where we would dynamically create a context would be if you add a new snippet language which hopefully is not that common. The problem with this approach is that we have a setting for custom snippet dir. This setting is not available until Talon is ready. The implementation in this pull request might be the the best solution available at the moment since the language csv file can be read immediately at start. |
make_sure_settings_file_exists() | ||
|
||
|
||
@resource.watch(settings_filepath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this handle if the file doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last time I tested the code, it created the file if it did not exist.
To me, it sounds like we might be better off keeping the language ids in python. Maybe we should instead clean up the extension definition stuff e.g. using a data class to define the list of extensions/associated spoken forms to make it more easily understandable/modifiable, and live with the need to branch this particular file for performance reasons. Plus, the alternative solutions to this PR require a talon restart if we modify the file, which is downer. |
In the community session today we talked further on this topic. We think we should probably have a new python file We want to have these language definitions typed and something like this could probably work: string_or_list = str | list[str]
lang = tuple[str, string_or_list] | tuple[str, string_or_list, string_or_list]
languages: list[lang] = [
("python", ["python"]),
("java", "java"),
("javascript", ["javascript"], ["something something"]),
] |
Migrated to an issue (#1585). |
No description provided.